-
Notifications
You must be signed in to change notification settings - Fork 275
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use rustix instead of libc #878
Conversation
Would it make sense to implement this as an alternative behind a feature flag rather than a direct replacement? Doing this would make it more convenient to compare any performance differences and behavior changes. |
I don't think so. The question comes down to how we expose this as a feature flag.
It adds a lot more complexity than just using one backend does. |
One approach to mutually exclusive feature flags like this is to use My context on this is as a maintainer of Ratatui, where we generally suggest people choose Crossterm as their backend. I'd like to get a feel for whether this change has actual performance benefits, and I'm sure that being able to measure that would be nice. Doing things with alternative implementations instead of pure replacement also carries less inherent risk - especially when there's no tests. |
Closes crossterm-rs#847 rustix is a wrapper around either raw Linux system calls or libc, based on the current platform. The main advantage is that it can make programs much more efficient, since system calls can be inlined directly into the functions that call them. I've seen rustix reduce instruction counts in my programs when I've made the switch in another programs. In addition, it reduces the amount of unsafe code. Signed-off-by: John Nunley <[email protected]>
If the maintainer wants a feature flag I will add it. However I think that it will add a lot of complexity where none existing before. Ideally, such a flag would be deprecated by the v1.0 release of crossterm, after we've decided to go one way or another. |
By definition, stable (
This sounds like a fantastic benefit, that I'd like to be able to measure.
I want to make it clear that I'm not a crossterm maintainer, just someone that has an interest in making sure that crossterm succeeds (Ratatui apps account for ~15-25% of Crossterm's monthly download metrics). Crossterm releases fairly slowly due to the availability of the maintainer's time. This adds weight to the idea of doing things in ways that don't require interventions down the line to address bugs or even features that work differently. The introduction of the Release and Repeat event kinds is an example of where this didn't happen, which regularly causes problems for users (so much so we leave a pinned issue at the top of the Ratatui issues page about it). Can you say with certainty that the rustix implementation (both your code, and the underlying crate) is bug free, and behavior equivalent to the existing libc code? (Maybe the answer to this is yes - like there's a test harness that proves this out in rustix or something). I'd strongly prefer to see a change like this put at least initially behind a feature flag so that it can be tested intentionally. The default can easily be set to use rustix, but having a possibility to use libc when testing or when bugs are found is important. I'd suggest to implement this by putting the libc impl behind a flag for 0.28, and then dropping the libc implementation in 0.29 assuming all goes well. |
Signed-off-by: John Nunley <[email protected]>
Signed-off-by: John Nunley <[email protected]>
Ok, I've made it so rustix is opt-in by a feature flag for now. |
Over in #892, I redid this (taking many cues from your implementation, but instead of wrapping / changing all the libc code, I made all the changes additive in an additive fashion. By default the rustix versions of code will be run, but adding the libc feature instead flips that to use the libc code paths. This approach makes sure there's no modification to the libc code, and makes it super easy to remove the libc code at any point. I think this has pretty much the same effect as your code (please correct me if I've missed anything). Does this help make a bit more sense of why I saw this as the less complex approach compared to replacing the implementation? |
No that's actually way better than my approach. Thanks for doing that! I'll close this PR in favor of that one. |
Closes #847
rustix is a wrapper around either raw Linux system calls or libc, based
on the current platform. The main advantage is that it can make programs
much more efficient, since system calls can be inlined directly into
the functions that call them. I've seen rustix reduce instruction counts
in my programs when I've made the switch in another programs.
In addition, it reduces the amount of unsafe code.